Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gRPC-Web client and server #695

Merged
merged 8 commits into from Jan 9, 2020
Merged

gRPC-Web client and server #695

merged 8 commits into from Jan 9, 2020

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Dec 14, 2019

Fixes #99

Implementation of https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-WEB.md for ASP.NET Core server and .NET Core gRPC client.

src/Shared/CommonGrpcProtocolHelpers.cs Show resolved Hide resolved
src/Grpc.AspNetCore.Web/GrpcWebOptions.cs Outdated Show resolved Hide resolved
src/Grpc.AspNetCore.Web/GrpcWebServiceExtensions.cs Outdated Show resolved Hide resolved
src/Grpc.AspNetCore.Web/Internal/GrpcWebFeature.cs Outdated Show resolved Hide resolved
src/Grpc.AspNetCore.Web/Internal/GrpcWebMiddleware.cs Outdated Show resolved Hide resolved
src/Grpc.AspNetCore.Web/Internal/GrpcWebMiddleware.cs Outdated Show resolved Hide resolved
src/Grpc.AspNetCore.Web/Internal/GrpcWebProtocolHelpers.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@JunTaoLuo JunTaoLuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still taking a look at the base64 pipereader/writer and request/responsestream

@brmdbr
Copy link

brmdbr commented Dec 30, 2019

I've build it from the sources to use in my Blazor client side app and I get the following error: WASM: Status(StatusCode=Internal, Detail="Error starting gRPC call: Cannot invoke method because it was wiped. See stack trace for details.")

Is this a problem related to this pr or am I implementing something wrong?

@JamesNK
Copy link
Member Author

JamesNK commented Dec 30, 2019

Instead of passing HttpClientHandler to GrpcWebHandler, create WasmHttpMessageHandler. There seems to be a bug in Blazor that its not public. You can do this:

var wasmHttpMessageHandlerType = Assembly.Load("WebAssembly.Net.Http").GetType("WebAssembly.Net.Http.HttpClient.WasmHttpMessageHandler");
var wasmHttpMessageHandler = (HttpMessageHandler)Activator.CreateInstance(wasmHttpMessageHandlerType);

dotnet/aspnetcore#17115

@SteveSandersonMS

@brmdbr
Copy link

brmdbr commented Dec 30, 2019

Thanks @JamesNK, that seems to work, I no longer get the cannot invoke error. I get another error now: WASM: Status(StatusCode=Internal, Detail="Error starting gRPC call: Buffer is not large enough for header")

Using this code:

var wasmHttpMessageHandlerType = Assembly.Load("WebAssembly.Net.Http").GetType("WebAssembly.Net.Http.HttpClient.WasmHttpMessageHandler");
var wasmHttpMessageHandler = (HttpMessageHandler)Activator.CreateInstance(wasmHttpMessageHandlerType);
var httpClient = new HttpClient(new GrpcWebHandler(GrpcWebMode.GrpcWebText, wasmHttpMessageHandler));
var channel = GrpcChannel.ForAddress("https://localhost:5001", new GrpcChannelOptions
            {
                HttpClient = httpClient
            });
var client = new Protos.LessonService.LessonServiceClient(channel);
var lesson = await client.GetLessonAsync(new Protos.GetLessonRequest { AllowedChars = "asdfghjkl;qwertyuiop", NumberOfWords = 15, Seed = 1337 });

Any idea on this? Again I'm not sure if its related to the pr as I'm implementing in a similar way as the tests in the .web projects.

@JamesNK
Copy link
Member Author

JamesNK commented Dec 30, 2019

Ohh, I forgot that I modified the client slightly.

You'll need to also compile and use Grpc.Net.Client.

@brmdbr
Copy link

brmdbr commented Jan 1, 2020

Ohh, I forgot that I modified the client slightly.

You'll need to also compile and use Grpc.Net.Client.

That fixed it! Thanks a lot.

@SteveSandersonMS
Copy link

SteveSandersonMS commented Jan 2, 2020

Instead of passing HttpClientHandler to GrpcWebHandler, create WasmHttpMessageHandler. There seems to be a bug in Blazor that its not public

Thanks - will be fixed with dotnet/blazor#1960 (as in, HttpClientHandler will just work out of the box and you won't have to do any weird stuff involving WasmHttpMessageHandler).

Copy link
Contributor

@JunTaoLuo JunTaoLuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments for the request/responsestreams. I'll finish up with the pipewriter/reader tomorrow.

Copy link
Contributor

@JunTaoLuo JunTaoLuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good but I'd like for pipe experts to take a look at the Base64PipeWriter and Base64PipeReader as well. @halter73 @jkotalik

Copy link
Contributor

@JunTaoLuo JunTaoLuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@halter73 @jkotalik can you guys take a look at the pipereader/writer logic as well?

Copy link
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial set of comments, overall things are looking good but let's discuss the next steps in our sync-up.
Sorry for the delay.

src/Grpc.AspNetCore.Web/README.md Outdated Show resolved Hide resolved
kokoro/build_nuget.sh Outdated Show resolved Hide resolved
src/Grpc.AspNetCore.Web/README.md Show resolved Hide resolved
src/Grpc.AspNetCore.Web/README.md Outdated Show resolved Hide resolved
src/Grpc.AspNetCore.Web/Internal/GrpcWebMode.cs Outdated Show resolved Hide resolved
/// Written to as closely as possible mirror the behaviour of the C++ implementation in grpc/grpc-web:
/// https://github.com/grpc/grpc-web/blob/92aa9f8fc8e7af4aadede52ea075dd5790a63b62/net/grpc/gateway/examples/echo/echo_service_impl.cc
/// </summary>
public class EchoService : Grpc.Gateway.Testing.EchoService.EchoServiceBase
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like you have a copy of the same service at testassets/FunctionalTestsWebsite/Services/EchoService.cs?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. That's so it is tested by CI

test/FunctionalTests/Infrastructure/GrpcTestFixture.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@jtattermusch jtattermusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good enough as a preview implementation of the grpc-web functionality. LGTM, thanks for incorporating all the comments.

This will be in the next release as preview for people to test it out and provide feedback/report issues, but there won't be "stable" nuget packages yet.

The next steps and release criteria for official release will be driven by grpc/proposal#169.

@JamesNK JamesNK merged commit bee59db into grpc:master Jan 9, 2020
@JamesNK JamesNK deleted the jamesnk/grpcweb4 branch January 9, 2020 19:04
@JamesNK
Copy link
Member Author

JamesNK commented Jan 9, 2020

🎈 🎉 🍰

@johanbrandhorst
Copy link

Hi, great work @JamesNK. Would you be interested in adding this implementation to the grpc-web compatibility tests I'm running? See https://github.com/johanbrandhorst/grpc-web-compatibility-test

@muraray
Copy link

muraray commented Feb 10, 2020

That's one hell of a serious pull request... grpc port to .net core paradigm, very much appreciate your work, thanks a ton... looking forward using it in our next project...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for grpc web
10 participants